feat: consolidate tool logic into RooTool base class (fixes #7822) #7824
Conversation
- Created abstract RooTool base class to consolidate tool logic - Refactored WriteToFileTool to extend RooTool - Created ToolRegistry for centralized tool management - This addresses issue #7822 to consolidate tool definitions
- Created ToolAdapter to bridge existing code with new RooTool system - Allows gradual migration of tools without breaking existing functionality - Provides fallback to legacy implementations for unmigrated tools
dosubot
bot
added
the
size:XL
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| ): Promise<void> { | ||
| // For now, we'll delegate to the existing implementation | ||
| // In a full refactor, we would move all the logic here | ||
| const { writeToFileTool } = await import("../writeToFileTool") |
There was a problem hiding this comment.
Is this intentional? The WriteToFileTool is supposed to consolidate all logic but it's just delegating back to the legacy implementation. This defeats the purpose of the refactoring - we should either move all the logic here or clearly mark this as a transitional state with a TODO.
| import { Task } from "../../task/Task" | ||
| import { | ||
| ToolUse, | ||
| ToolResponse, |
There was a problem hiding this comment.
The ToolResponse import appears to be unused. Could we remove it to keep the imports clean?
| * Get tool description map for prompts | ||
| * This replaces the old toolDescriptionMap | ||
| */ | ||
| getToolDescriptionMap(): Record<string, (args: any) => string | undefined> { |
There was a problem hiding this comment.
Could we improve type safety here by using ToolArgs instead of any?
| getToolDescriptionMap(): Record<string, (args: any) => string | undefined> { | |
| getToolDescriptionMap(): Record<string, (args: ToolArgs) => string | undefined> { | |
| const descriptionMap: Record<string, (args: ToolArgs) => string | undefined> = {} |
| const displayNames: Record<string, string> = {} | ||
|
|
||
| // Map tool names to display names | ||
| const nameMapping: Record<ToolName, string> = { |
There was a problem hiding this comment.
This duplicates the TOOL_DISPLAY_NAMES constant from shared/tools.ts. Could we import and reuse the existing constant to avoid duplication and potential sync issues?
| return fetchInstructionsTool(cline, block, askApproval, handleError, pushToolResult) | ||
| } | ||
| default: | ||
| throw new Error(`Unknown tool: ${toolName}`) |
There was a problem hiding this comment.
Could we create a specific error class like UnknownToolError for better error handling and debugging? Generic errors make it harder to track down issues in production.
| */ | ||
| private registerTools(): void { | ||
| // Register each tool implementation | ||
| this.registerTool(new WriteToFileTool()) |
There was a problem hiding this comment.
Would it be helpful to add a TODO comment here tracking which tools still need migration? Something like:
| this.registerTool(new WriteToFileTool()) | |
| // Register each tool implementation | |
| this.registerTool(new WriteToFileTool()) | |
| // TODO: Migrate remaining tools (21 remaining): | |
| // Priority 1 (Core editing): | |
| // - ReadFileTool | |
| // - ApplyDiffTool | |
| // - ExecuteCommandTool | |
| // Priority 2 (Search/Navigation): | |
| // - SearchFilesTool | |
| // - ListFilesTool | |
| // - ListCodeDefinitionNamesTool | |
| // - CodebaseSearchTool | |
| // Priority 3 (Other): | |
| // ... etc |
hannesrudolph
added
the
Issue/PR - Triage
hannesrudolph
added
PR - Needs Preliminary Review
and removed
Issue/PR - Triage
|
This PR is incompletem the issue needs scoping |
github-project-automation
bot
moved this from PR [Needs Prelim Review]
to Done
in Roo Code Roadmap
Summary
This PR addresses Issue #7822 by introducing a new architecture for tool definitions that consolidates all tool-related logic into a single class structure.
Changes
New Architecture Components
RooTool Base Class (
src/core/tools/base/RooTool.ts)WriteToFileTool Implementation (
src/core/tools/implementations/WriteToFileTool.ts)ToolRegistry (
src/core/tools/ToolRegistry.ts)ToolAdapter (
src/core/tools/ToolAdapter.ts)Benefits
Migration Strategy
Next Steps
Testing
Feedback and guidance are welcome on this architectural approach before proceeding with the full migration.
Closes #7822
Important
Consolidates tool logic into
RooToolbase class, introducesToolRegistryfor centralized management, and migratesWriteToFileToolto new architecture.RooToolbase class inRooTool.tsfor consolidating tool logic.ToolRegistryinToolRegistry.tsfor centralized tool management.ToolAdapterinToolAdapter.tsfor backward compatibility with legacy tool invocations.WriteToFileToolto extendRooTool, consolidating its logic inWriteToFileTool.ts.ToolAdapterfor compatibility.This description was created by
for 4d97f77. You can customize this summary. It will automatically update as commits are pushed.